Feature findings enhancemnt #1427
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Riya Saxena <[email protected]>
Signed-off-by: Riya Saxena <[email protected]>
riysaxen-amzn
requested review from
lezzago,
AWSHurneyt,
sbcd90,
eirsep,
getsaurabh02,
praveensameneni,
qreshi,
bowenlan-amzn,
rishabhmaurya and
engechas
as code owners
5 tasks
engechas
reviewed
Feb 20, 2024
Comment on lines
+124
to
+125
| QueryBuilders.boolQuery().mustNot( | ||
| QueryBuilders.prefixQuery("queries.id", "threat_intel_") |
There was a problem hiding this comment.
What's the reason to filter out the threat intel queries here? As a user I would expect the filter to be based on the provided detectionType
There was a problem hiding this comment.
Yes, I have categorized the detectionType parameter into two types: "threat" and normal rule based. If the frontend API calls the findings API with detectionType set to "threat," all threat intel-related findings will be generated with this query. I'm open to considering alternative solutions for this categorization.
There was a problem hiding this comment.
I think the enum comment I had on the other CR makes this a bit more clear. Having the arbitrary text input + looking for exactly 2 cases seems like it should be enum-based.
If we don't expect the detectionType options to expand, then having a boolean field seems more user-friendly to me. Something like getThreatIntelFindingsOnly (name could use some work) that allows the user to pick which type of findings are returned.
Or maybe a new API, say GetThreatIntelFindings, would also make sense to me.
Another thought on this - this change is arguably a breaking one. We are changing the default behavior of an existing API to now filter out the threat intel findings. If a user has a system that relies on this API already and they are expecting threat intel findings, then it would break their use case.
Is there a way we can explore doing the filtering on the frontend, or at least leaving the default behavior as-is? I'm a bit concerned with merging this at the moment, but open to others' thoughts.
There was a problem hiding this comment.
Missed line 128.. not a breaking change or change to default behavior since filtering is only applied if a detectionType is supplied
Signed-off-by: Riya Saxena <[email protected]>
Signed-off-by: Riya Saxena <[email protected]>
engechas
approved these changes
Mar 9, 2024
AWSHurneyt
approved these changes
Mar 9, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Mar 9, 2024
* added support for param in Finding API Signed-off-by: Riya Saxena <[email protected]> * added detectionType as param for Findings API enhancements Signed-off-by: Riya Saxena <[email protected]> * added searchString param in FIndingsAPI Signed-off-by: Riya Saxena <[email protected]> * adding addiional params findingIds, startTime and endTime Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> (cherry picked from commit 2420c2c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
praveensameneni
pushed a commit
that referenced
this pull request
Mar 10, 2024
* added support for param in Finding API * added detectionType as param for Findings API enhancements * added searchString param in FIndingsAPI * adding addiional params findingIds, startTime and endTime --------- (cherry picked from commit 2420c2c) Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
eirsep
reviewed
Mar 12, 2024
| table | ||
| table, | ||
| severity, | ||
| detectionType |
There was a problem hiding this comment.
we can't use this variable .. this is not a keyword in alerting domain.. if it's for security analytics we need to translate it into a jargon or query searchString which is generalized to Alerting plugin?
jowg-amazon
pushed a commit
that referenced
this pull request
Mar 15, 2024
* added support for param in Finding API * added detectionType as param for Findings API enhancements * added searchString param in FIndingsAPI * adding addiional params findingIds, startTime and endTime --------- (cherry picked from commit 2420c2c) Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 task
jowg-amazon
pushed a commit
that referenced
this pull request
Mar 15, 2024
* added support for param in Finding API Signed-off-by: Riya Saxena <[email protected]> * added detectionType as param for Findings API enhancements Signed-off-by: Riya Saxena <[email protected]> * added searchString param in FIndingsAPI Signed-off-by: Riya Saxena <[email protected]> * adding addiional params findingIds, startTime and endTime Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]>
jowg-amazon
added a commit
that referenced
this pull request
Mar 15, 2024
* Feature findings enhancemnt (#1427) (#1457) * added support for param in Finding API * added detectionType as param for Findings API enhancements * added searchString param in FIndingsAPI * adding addiional params findingIds, startTime and endTime --------- (cherry picked from commit 2420c2c) Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Findings API Enhancements changes and integ tests fix (#1464) (#1474) * solution to fix integ tests Signed-off-by: Riya Saxena <[email protected]> * fix flaky DocumentMonitor Runner tests Signed-off-by: Riya Saxena <[email protected]> * fix findings API enhancemnts Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> (cherry picked from commit ba84d04) * fix integ tests Signed-off-by: Joanne Wang <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Joanne Wang <[email protected]> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Riya <[email protected]>
AWSHurneyt
added a commit
that referenced
this pull request
Mar 15, 2024
…tion message ctx. (#1450) (#1480) * [Backport 2.x] Enhance per bucket, and per document monitor notification message ctx. (#1450) (#1477) * Enhance per bucket, and per document monitor notification message ctx. (#1450) * Adding dev logs. Signed-off-by: AWSHurneyt <[email protected]> * Added support for returning sample documents for bucket level monitors. Signed-off-by: AWSHurneyt <[email protected]> * Added support for printing query/rule info in notification messages. Signed-off-by: AWSHurneyt <[email protected]> * Extracted out helper function. Signed-off-by: AWSHurneyt <[email protected]> * Extracted out helper function. Signed-off-by: AWSHurneyt <[email protected]> * Added support for printing document data in notification messages for document level monitors. Signed-off-by: AWSHurneyt <[email protected]> * Refactored logic after making AlertContext a separate class from Alert instead of inheriting/extending it in common utils. Signed-off-by: AWSHurneyt <[email protected]> * Moved AlertContext data model from common utils to alerting plugin. Signed-off-by: AWSHurneyt <[email protected]> * Fixed ktlint errors. Signed-off-by: AWSHurneyt <[email protected]> * Added additional unit tests. Signed-off-by: AWSHurneyt <[email protected]> * Extracted sample doc aggs logic into helper function. Added support for sorting sample docs based on metric aggregations. Signed-off-by: AWSHurneyt <[email protected]> * Extracted get sample doc logic into helper function. Added sorting for sample docs. Signed-off-by: AWSHurneyt <[email protected]> * Removed dev code. Signed-off-by: AWSHurneyt <[email protected]> * Fixed ktlint errors. Signed-off-by: AWSHurneyt <[email protected]> * Added comments based on PR feedback. Signed-off-by: AWSHurneyt <[email protected]> * Added logic to make mGet calls in batches. Signed-off-by: AWSHurneyt <[email protected]> --------- Signed-off-by: AWSHurneyt <[email protected]> (cherry picked from commit 5dc690c) Signed-off-by: AWSHurneyt <[email protected]> * Fixed imports. Signed-off-by: AWSHurneyt <[email protected]> --------- Signed-off-by: AWSHurneyt <[email protected]> * [Backport 2.11] Backport #1427 and #1464 to 2.11 (#1479) * Feature findings enhancemnt (#1427) (#1457) * added support for param in Finding API * added detectionType as param for Findings API enhancements * added searchString param in FIndingsAPI * adding addiional params findingIds, startTime and endTime --------- (cherry picked from commit 2420c2c) Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * Findings API Enhancements changes and integ tests fix (#1464) (#1474) * solution to fix integ tests Signed-off-by: Riya Saxena <[email protected]> * fix flaky DocumentMonitor Runner tests Signed-off-by: Riya Saxena <[email protected]> * fix findings API enhancemnts Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> (cherry picked from commit ba84d04) * fix integ tests Signed-off-by: Joanne Wang <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Joanne Wang <[email protected]> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Riya <[email protected]> * [Backport 2.x] Enhance per bucket, and per document monitor notification message ctx. (#1450) (#1477) * Enhance per bucket, and per document monitor notification message ctx. (#1450) * Adding dev logs. Signed-off-by: AWSHurneyt <[email protected]> * Added support for returning sample documents for bucket level monitors. Signed-off-by: AWSHurneyt <[email protected]> * Added support for printing query/rule info in notification messages. Signed-off-by: AWSHurneyt <[email protected]> * Extracted out helper function. Signed-off-by: AWSHurneyt <[email protected]> * Extracted out helper function. Signed-off-by: AWSHurneyt <[email protected]> * Added support for printing document data in notification messages for document level monitors. Signed-off-by: AWSHurneyt <[email protected]> * Refactored logic after making AlertContext a separate class from Alert instead of inheriting/extending it in common utils. Signed-off-by: AWSHurneyt <[email protected]> * Moved AlertContext data model from common utils to alerting plugin. Signed-off-by: AWSHurneyt <[email protected]> * Fixed ktlint errors. Signed-off-by: AWSHurneyt <[email protected]> * Added additional unit tests. Signed-off-by: AWSHurneyt <[email protected]> * Extracted sample doc aggs logic into helper function. Added support for sorting sample docs based on metric aggregations. Signed-off-by: AWSHurneyt <[email protected]> * Extracted get sample doc logic into helper function. Added sorting for sample docs. Signed-off-by: AWSHurneyt <[email protected]> * Removed dev code. Signed-off-by: AWSHurneyt <[email protected]> * Fixed ktlint errors. Signed-off-by: AWSHurneyt <[email protected]> * Added comments based on PR feedback. Signed-off-by: AWSHurneyt <[email protected]> * Added logic to make mGet calls in batches. Signed-off-by: AWSHurneyt <[email protected]> --------- Signed-off-by: AWSHurneyt <[email protected]> (cherry picked from commit 5dc690c) Signed-off-by: AWSHurneyt <[email protected]> * Fixed imports. Signed-off-by: AWSHurneyt <[email protected]> --------- Signed-off-by: AWSHurneyt <[email protected]> * Fixed test. Signed-off-by: AWSHurneyt <[email protected]> * Fixed ktlint error. Signed-off-by: AWSHurneyt <[email protected]> --------- Signed-off-by: AWSHurneyt <[email protected]> Signed-off-by: Riya Saxena <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Joanne Wang <[email protected]> Co-authored-by: Joanne Wang <[email protected]> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Riya <[email protected]>
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Mar 25, 2024
* added support for param in Finding API Signed-off-by: Riya Saxena <[email protected]> * added detectionType as param for Findings API enhancements Signed-off-by: Riya Saxena <[email protected]> * added searchString param in FIndingsAPI Signed-off-by: Riya Saxena <[email protected]> * adding addiional params findingIds, startTime and endTime Signed-off-by: Riya Saxena <[email protected]> --------- Signed-off-by: Riya Saxena <[email protected]> (cherry picked from commit 2420c2c) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#795
Added detectionType severity and searchString as params in Findings API
CheckList:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.